Authorization refactor in preparation for fine-grained authorization #12313

markylaing · 2023-09-25T14:57:42Z

This is a large refactor of authorization in LXD. The key points are:

There is no longer a concept of an "admin" outside of the auth package.
All EndpointActions must have either an AccessHandler or AllowUntrusted must be true.
The auth package now contains Entitlements which act or particular objects. Access handlers should ask "Does user X have entitlement Y on object Z?"

This is in preparation for https://warthogs.atlassian.net/browse/LXD-389, the remainder of which will be found in #12252

markylaing · 2023-09-25T16:41:37Z

I think the DCO check is failing because there are too many commits. I've definitely signed them all. If it's an issue I'll figure out how to run the action locally and submit a fix upstream.

lxd/auth/authorization_types.go

lxd/auth/driver_tls.go

lxd/auth/authorization_types.go

lxd/auth/authorization_objects.go

lxd/auth/driver_tls.go

lxd/daemon.go

lxd/db/operationtype/operation_type.go

lxd/project/permissions.go

lxd/instances.go

lxd/storage_buckets.go

lxd/warnings.go

markylaing · 2023-09-27T13:42:28Z

@tomponline @monstermunchkin @gabrielmougard @MusicDin I've addressed the comments so ready for round 2 when you have a moment. Thanks :)

markylaing · 2023-09-27T19:14:38Z

I've just pushed an update because my most recent changes still included some logic which caused inconsistent behaviour in in the OpenFGA driver. Tests are passing locally including RBAC tests.

lxd/auth/authorization_objects.go

markylaing · 2023-10-04T11:10:51Z

@tomponline Regarding the problem with the events websocket I mentioned on Monday.

If you add this commit: f02bec6

Rather than fail because the user does not have sufficient privilege to view logging events, the test suite will hang indefinitely, never outputting an event. This happens because the client implementation of GetEvents does not specify an event type, so the /1.0/events endpoint returns all events to which the user has access. The CLI adds a handler to the EventListener returned by GetEvents, but since no logging events are ever returned, the handler is never run.

Really this should return a 403 Forbidden, but I'm not sure how this can be achieved with the current client implementation, especially given that we really don't want to be changing the client interface. Given that this behaviour is currently in main with TLS and RBAC, I don't think the OpenFGA driver needs to fix it. We can fix at a later date.

markylaing · 2023-10-04T11:23:28Z

@tomponline an additional pain I have found is that lxc exec requires access to /1.0/events. Rather than querying the /1.0/operations/{uuid}/wait endpoint, the client uses an event listener to figure out when an operation has completed. This means that for a user to be able to exec into a single instance they currently need to be able to see all operation and lifecycle events. This is a fairly easy fix though, I'll point it out in the other PR in a commit message.

tomponline · 2023-10-05T12:44:56Z

This is a fairly easy fix though, I'll point it out in the other PR in a commit message.

Can you put this up as a standalone PR so we can evaluate it independently, it sounds like a reasonable change to me though. Thanks

tomponline · 2023-10-05T12:46:30Z

Rather than fail because the user does not have sufficient privilege to view logging events, the test suite will hang indefinitely, never outputting an event. This happens because the client implementation of GetEvents does not specify an event type, so the /1.0/events endpoint returns all events to which the user has access. The CLI adds a handler to the EventListener returned by GetEvents, but since no logging events are ever returned, the handler is never run.

Really this should return a 403 Forbidden,

Why do you think it should be a 403?

Isn't this like the other discussion we had the other day regarding filtered lists, whereby if you dont have permission on any resources you just get an empty output rather than an error? In this case, isn't a blocking event stream with no events equivalent to that?

markylaing · 2023-10-05T12:55:39Z

Why do you think it should be a 403?

Isn't this like the other discussion we had the other day regarding filtered lists, whereby if you dont have permission on any resources you just get an empty output rather than an error? In this case, isn't a blocking event stream with no events equivalent to that?

I guess it is equivalent. Though in this case I think the reason for a hanging lxc monitor is a little more obfuscated. Happy to leave as it is for now though.

markylaing · 2023-10-05T12:56:03Z

This is a fairly easy fix though, I'll point it out in the other PR in a commit message.

Can you put this up as a standalone PR so we can evaluate it independently, it sounds like a reasonable change to me though. Thanks

Yep will do.

tomponline · 2023-10-05T13:25:25Z

Can we make it so if the user has access to no events it returns 403 then?

markylaing · 2023-10-05T16:29:11Z

This is a fairly easy fix though, I'll point it out in the other PR in a commit message.

Can you put this up as a standalone PR so we can evaluate it independently, it sounds like a reasonable change to me though. Thanks

Yep will do.

#12349

Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing · 2023-10-25T08:42:55Z

Rebased.

tomponline · 2023-10-25T09:41:13Z

lxd/auth/driver_tls.go

+
+	authenticationProtocol := details.authenticationProtocol()
+	if authenticationProtocol != api.AuthenticationMethodTLS {
+		t.logger.Warn("Authentication protocol is not compatible with authorization driver", logger.Ctx{"protocol": authenticationProtocol})


I wonder if we should ban all requests that use an invalid authentication protocol rather than fail open?

I wasn't sure. Currently if you authenticate with Candid (without RBAC) or OIDC the default is to allow everything. I can change it but will need to change some tests. Any existing users using these authentication methods will find that they are unauthorized and will need to configure authorization via the unix socket.

I think it's unlikely that there many (or any) users using OIDC or standalone Candid though.

I see, so they dont have a trusted client TLS cert because they authenticated through a different mechanism but the active authorizer is for TLS. I think we should keep it the same, but perhaps clarify that section there with a comment how you explained it above.

tomponline · 2023-10-25T09:51:51Z

lxd/auth/driver_rbac.go

-	r.resourcesLock.Lock()
-	r.resources = resourcesMap
-	r.resourcesLock.Unlock()
+	if shared.ValueInSlice(PermissionAdmin, permissions[""]) {


@markylaing I dont understand what this part is doing, will it always be false?

The permissions map here is a map of project name to slice of permissions. However, if the user is an admin, this permission is stored in the map with the project name as an empty string. This is how it is currently done, but I can change it to something like:

type rbacPermission struct { admin bool projects map[string][]Permission }

The permission cache defined on rbac will then be a map[string]rbacPermission where the keys are usernames.

How does that sound?

yeah i think that would be clearer. Thanks

tomponline · 2023-10-25T09:53:20Z

lxd/auth/driver_rbac.go

-	return nil
+	if details.isAllProjectsRequest {
+		// Only admins can use the all-projects parameter.
+		return nil, api.StatusErrorf(http.StatusForbidden, "User is not an administrator")


Is "administrator" an RBAC concept? As I remember you were removing IsAdmin concept from LXD.
So I wouldn't want to see "administrator" in the error unless it was related to an RBAC concept specifically.

Yes "admin" is an RBAC permission which grants full access. I'll update the comment to: // Only RBAC administrators can use the all-projects parameter.

tomponline · 2023-10-25T09:54:54Z

lxd/auth/driver_rbac.go

+					return
+				}
+
+				logger.Errorf("Failed to prepare RBAC query: %v", err)


It would be good to include in the log message what it was trying to do, i.e "Failed RBAC status check, failed preparing request: %v"

tomponline · 2023-10-25T09:55:43Z

lxd/auth/driver_rbac.go

+					continue
+				}
+
+				logger.Errorf("Failed to connect to RBAC, re-trying: %v", err)


Something like "Failed RBAC status check, failed connecting to RBAC service, retrying: %v"

tomponline · 2023-10-25T09:56:07Z

lxd/auth/driver_rbac.go


-		// Ignore unknown projects.
-	}
+			if resp.StatusCode != 200 {


Should use constants from http package here.

tomponline · 2023-10-25T09:56:11Z

lxd/auth/driver_rbac.go

-			access.Projects[projectName] = v
-			break
-		}
+			if resp.StatusCode == 504 {


Should use constants from http package here.

tomponline · 2023-10-25T10:10:12Z

lxd/daemon.go

-		d.authorizer.StopStatusCheck()
+		err := d.authorizer.StopService(d.shutdownCtx)
+		if err != nil {
+			logger.Error("Failed to stop authorizer service", logger.Ctx{"error": err})


We should return an error here rather than logging right? Otherwise we can get into an inconsistent state?

Or do we also need to fallback to TLS in that case?

tomponline · 2023-10-25T10:15:34Z

lxd/daemon.go

@@ -1803,14 +1803,17 @@ func (d *Daemon) setupRBACServer(rbacURL string, rbacKey string, rbacExpiry int6
 	var err error


I think in general we should rework this function to be a generic "one of the authorization keys has changed, we need to restart/change the authorization driver" and have it handle all the driver's config keys so it can fallback to the previous active driver (and not just TLS) on error. But this can come as a separate PR.

tomponline · 2023-10-25T10:20:18Z

lxd/db/cluster/entities.go

+
+		spaceSeparatedEntityPath := strings.Replace(entityPath, "/", " / ", -1)
+
+		// Make an []any for the number of expected path arguments and set each value in the slice to a *string.


why not []string or []*string?

tomponline · 2023-10-25T10:23:30Z

lxd/project/permissions.go

 		}

-		if !authorizer.UserHasPermission(r, projectName, "view") {
+		err = authorizer.CheckPermission(r.Context(), r, object, auth.EntitlementCanView)


Can we avoid calling this function for each entity in entries - this could be a lot of them and slow?
Can we use the transactional checker concept we discussed?

tomponline · 2023-10-25T10:29:12Z

lxd/api_metrics.go

+		return response.SmartError(err)
+	} else if err != nil {
+		// This is counterintuitive. We are unauthorized to get a permission checker for viewing instances because a metric type certificate
+		// can't view instances. However, in order to get to this point we must already have auth.EntitlementCanViewMetrics. So we can view


So if you can view all metrics with auth.EntitlementCanViewMetrics why cant you also do filtering?

tomponline · 2023-10-25T10:31:28Z

lxd/certificates.go

@@ -528,7 +544,15 @@ func certificatesPost(d *Daemon, r *http.Request) response.Response {
 	}

 	// Handle requests by non-admin users.
-	if !s.Authorizer.UserIsAdmin(r) {
+	var userCanCreateCertificates bool


I think this section could do with a comment explaining whats going on here and why its special.

tomponline · 2023-10-25T10:33:47Z

lxd/images.go

-	trusted := d.checkTrustedClient(r) == nil && allowProjectPermission("images", "manage-images")(d, r) == response.EmptySyncResponse
+	projectName := request.ProjectParam(r)
+
+	var userCanCreateImages bool


A comment here explaining why this bit is special would be good.

tomponline · 2023-10-25T10:35:03Z

lxd/images.go

@@ -1137,6 +1147,12 @@ func imagesPost(d *Daemon, r *http.Request) response.Response {
 			return fmt.Errorf("Failed syncing image between nodes: %w", err)
 		}

+		// Add the image to the authorizer.
+		err = s.Authorizer.AddImage(r.Context(), projectName, info.Fingerprint)


Can we do this earlier, fail if it fails, and if something goes wrong later remove the entry from the authorizer again?
This feels safer to me.

tomponline · 2023-10-25T10:40:08Z

lxd/instances_put.go

 	var names []string
 	var instances []instance.Instance
 	for _, inst := range c {
 		if inst.Project().Name != projectName {
 			continue
 		}

+		// Only allow changing the state of instances the user has permission for.
+		if !userHasPermission(auth.ObjectInstance(inst.Project().Name, inst.Name())) {
+			continue


Think this should be an error.

tomponline · 2023-10-25T10:42:11Z

lxd/instance/drivers/driver_lxc.go

@@ -327,6 +327,12 @@ func lxcCreate(s *state.State, args db.InstanceArgs, p api.Project) (instance.In
 	if d.isSnapshot {
 		d.state.Events.SendLifecycle(d.project.Name, lifecycle.InstanceSnapshotCreated.Event(d, nil))
 	} else {
+		// Add instance to authorizer.
+		err = d.state.Authorizer.AddInstance(d.state.ShutdownCtx, d.project.Name, d.Name())


I think in general we need to review and consider the ordering of when entities are added/removed from the authorizer and whether if that fails whether it should be passed back to the user.

tomponline · 2023-10-25T10:45:35Z

lxd/operations.go

+						return response.InternalError(fmt.Errorf("Unable to create authorization object for operation: %w", err))
+					}
+
+					err = s.Authorizer.CheckPermission(r.Context(), r, object, entitlement)


Can we use a "get permission checker" to avoid having to call out to the authorizer service for each op.Resources()?

tomponline

Although there's some additional conversations and changes to be made to this, in the interests of making progress I am approving it as I do not see any glaring problems with it. Thanks!

changes made

markylaing · 2023-10-25T11:12:08Z

Thanks for your review @tomponline. Glad it's finally merged 🥳

I've created an issue for the outstanding comments: #12461

markylaing self-assigned this Sep 25, 2023

markylaing force-pushed the authorization-refactor branch from 152c1c6 to c9c461a Compare September 25, 2023 16:33

markylaing marked this pull request as ready for review September 25, 2023 16:34

markylaing requested a review from tomponline as a code owner September 25, 2023 16:34

markylaing requested review from monstermunchkin, gabrielmougard and MusicDin September 25, 2023 16:35

markylaing commented Sep 25, 2023

View reviewed changes

lxd/auth/authorization_types.go Show resolved Hide resolved

markylaing commented Sep 25, 2023

View reviewed changes

lxd/auth/driver_tls.go Outdated Show resolved Hide resolved

monstermunchkin previously requested changes Sep 26, 2023

View reviewed changes

markylaing force-pushed the authorization-refactor branch 3 times, most recently from 52d4a3d to 42c5f30 Compare September 27, 2023 13:38

markylaing requested a review from monstermunchkin September 27, 2023 13:41

markylaing force-pushed the authorization-refactor branch from 42c5f30 to ee4ebdd Compare September 27, 2023 19:06

markylaing commented Sep 28, 2023

View reviewed changes

lxd/auth/authorization_objects.go Outdated Show resolved Hide resolved

markylaing force-pushed the authorization-refactor branch 2 times, most recently from c332af2 to 7e245ba Compare September 28, 2023 14:50

markylaing force-pushed the authorization-refactor branch from 7e245ba to 8375b91 Compare October 5, 2023 16:20

markylaing added 2 commits October 25, 2023 09:42

lxd/storage: Add/Remove/Rename storage volumes in authorizer.

3371cf9

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd: Update authorization for warnings.

7c9f699

Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing force-pushed the authorization-refactor branch from c4edd45 to 7c9f699 Compare October 25, 2023 08:42

tomponline reviewed Oct 25, 2023

View reviewed changes

lxd/auth/driver_rbac.go

// Ignore unknown projects.

}

if resp.StatusCode != 200 {

Copy link

Member

tomponline Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use constants from http package here.

tomponline reviewed Oct 25, 2023

View reviewed changes

tomponline approved these changes Oct 25, 2023

View reviewed changes

tomponline merged commit 20efe1b into canonical:main Oct 25, 2023
25 of 26 checks passed

markylaing mentioned this pull request Oct 25, 2023

Authorization drivers - follow up #12461

Closed

18 tasks

This was referenced Mar 14, 2024

Auth: Pre-check permissions when performing bulk state update. #13155

Merged

lxd: Improves efficiency of operation cancel with permission checker. #13156

Merged

markylaing mentioned this pull request Mar 28, 2024

Metrics: Differentiate between restricted and unrestricted certificates #13214

Merged

		@@ -1803,14 +1803,17 @@ func (d *Daemon) setupRBACServer(rbacURL string, rbacKey string, rbacExpiry int6
		var err error


		spaceSeparatedEntityPath := strings.Replace(entityPath, "/", " / ", -1)

		// Make an []any for the number of expected path arguments and set each value in the slice to a *string.

Authorization refactor in preparation for fine-grained authorization #12313

Authorization refactor in preparation for fine-grained authorization #12313

Conversation

markylaing commented Sep 25, 2023

markylaing commented Sep 25, 2023

markylaing commented Sep 27, 2023

markylaing commented Sep 27, 2023

markylaing commented Oct 4, 2023

markylaing commented Oct 4, 2023

tomponline commented Oct 5, 2023

tomponline commented Oct 5, 2023

markylaing commented Oct 5, 2023

markylaing commented Oct 5, 2023

tomponline commented Oct 5, 2023

markylaing commented Oct 5, 2023

markylaing commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

tomponline left a comment

Choose a reason for hiding this comment

markylaing commented Oct 25, 2023

tomponline Oct 25, 2023 •

edited

Loading

tomponline Oct 25, 2023 •

edited

Loading